federation: Add admission controller for policy-based placement #44786

tsandall · 2017-04-21T22:49:06Z

Here's the initial version of the scheduling policy admission controller. It's at the point where it would benefit from having another pair of eyes look at it. The main thing I'm unsure of is the serialization of Kube resources for the webhook/query call.

Release Note:

The federation-apiserver now supports a SchedulingPolicy admission controller that enables policy-based control over placement of federated resources.

Ref #39982

k8s-reviewable · 2017-04-21T22:49:12Z

This change is

k8s-ci-robot · 2017-04-21T22:49:14Z

Hi @tsandall. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

ghost · 2017-04-21T23:23:00Z

@k8s-bot ok to test

ghost · 2017-05-01T19:09:39Z

Replaced assignee @irfanurrehman with @nikhiljindal as per Federation Engineering Roadmap. I will also take a look now.

ghost

I did a quick review and in general the code looks great. As I'm going to be OOTO for 2.5 weeks, I'll leave a detailed review up to others so as not to hold things up. As soon as one other has reviewed the code in some detail, I'm happy to approve. @nikhiljindal Can you?

ghost · 2017-05-01T21:34:01Z

federation/plugin/pkg/admission/schedulingpolicy/admission.go

+	policyConfigMapNamespace = "kube-federation-scheduling-policy"
+	minRetryBackoff          = time.Millisecond
+	maxRetryBackoff          = time.Second * 60
+	defaultRetryBackoff      = time.Millisecond * 100


It would be good to add a comment explaining what the associated backoff algorithm is. Is it standard exponential? If so, what's the purpose of defaultRetryBackoff? etc.

Will update with comment. The admission controller itself is not implementing the backoff; it's done by one of the utilities in kube.

nikhiljindal · 2017-05-24T03:18:33Z

federation/plugin/pkg/admission/schedulingpolicy/admission.go

+	*admission.Handler
+	policyEngineClient       *rest.RESTClient            // client to communicate with policy engine
+	policyEngineRetryBackoff time.Duration               // backoff for policy engine queries
+	client                   internalclientset.Interface // client to communicate with federation-apiserve


apiserve -> apiserver

nikhiljindal · 2017-05-24T03:20:19Z

federation/plugin/pkg/admission/schedulingpolicy/admission.go

+}
+
+func (c *admissionController) Admit(a admission.Attributes) (err error) {
+


nit: These extra blank lines are not needed and can be removed

nikhiljindal · 2017-05-24T03:22:28Z

federation/plugin/pkg/admission/schedulingpolicy/admission.go

+const (
+	pluginName               = "SchedulingPolicy"
+	configKey                = "schedulingPolicy"
+	policyConfigMapNamespace = "kube-federation-scheduling-policy"


Didnt we decide to keep system in the name?
"federation-system-scheduling-policy" maybe?

Let me know what you want to do regarding the name. In kubernetes/community#292 this one was suggested and specified in the proposal.

ok, reread the discussion there, thanks for the link.
lg.

nikhiljindal · 2017-05-24T03:25:28Z

federation/plugin/pkg/admission/schedulingpolicy/admission.go

+		return
+	}
+
+	event := &api.Event{


Use versioned objects (v1.Event instead of api.Event)

It seems the internalclientset requires api rather than v1. Let me know how to proceed.

yes, see my comment above about using versionedclientset.
You will have to use internal API structs with internalclientset.

Consider this resolved.

nikhiljindal · 2017-05-24T03:25:52Z

federation/plugin/pkg/admission/schedulingpolicy/admission.go

+		Type: "Warning",
+	}
+
+	if _, err := c.client.Core().Events(a.GetNamespace()).Create(event); err != nil {


In our controllers we use EventRecorder. We can use the same here?

Not sure, I will look into this.

This would make the admission controller depend on the controller manager code. Is this desirable?

It seems the EventRecorder utility is not compatible with the internalclientset. Let me know how we should proceed.

nikhiljindal · 2017-05-24T03:53:20Z

federation/plugin/pkg/admission/schedulingpolicy/query.go

+	return fmt.Errorf("reason(s): %v", strings.Join(d.Errors, "; "))
+}
+
+func convertObject(obj runtime.Object, gv schema.GroupVersion) (interface{}, error) {


Its not clear to me what you are converting the object into and why?
Are you serializing the obj into json?

Also again, the code seems generic. Can we reuse existing code?

This was needed so that the admission controller could construct the policy query as follows:

{ "input": <resource> }

What I think would be better is if the <resource> value was not encapsulated inside {"input": ...}.

It should be fine to remove this entirely.

nikhiljindal · 2017-05-24T03:59:48Z

federation/plugin/pkg/admission/schedulingpolicy/query.go

+// executing a policy query.
+type policyEngineInput struct {
+	Input struct {
+		Resource interface{} `json:"resource"`


Resource can be runtime.Object?

nikhiljindal · 2017-05-24T04:00:29Z

federation/plugin/pkg/admission/schedulingpolicy/query.go

+	Errors   []string `json:"errors,omitempty"`
+	Resource struct {
+		Metadata struct {
+			Annotations map[string]string `json:"annotations,omitempty"`


I think its fine to keep Annotations as top level field. No need to nest it in Resource.Metadata

Also am wondering if it should just return the updated object.
That way we wont have to update the API when we move annotations to fields.

The concern I have with returning the entire object is it implies that the admission controller supports updates on arbitrary fields. Currently, this is not the case, it only supports annotations. Changing the admission controller to support arbitrary fields (e.g., annotations, spec.foo, status.bar[0].baz, etc.) would be tricky with the current admission controller framework.

I'm OK with changing the response to expect (i) errors and (ii) annotations for now. Let me know what you think.

Yes, on thinking more I agree with you. This way, we restrict what the policy engine can change. It cannot change anything apart from annotations on the object.
Infact, I think we can restrict it more. Instead of returning a whole map of annotations and we merging them all as is, it should only return cluster selector values.

This is fine as is for now.

nikhiljindal · 2017-05-24T04:06:02Z

federation/plugin/pkg/admission/schedulingpolicy/admission.go

+	return nil
+}
+
+func (c *admissionController) SetInternalKubeClientSet(client internalclientset.Interface) {


Why internal clientset and not the versioned clientset?

As far as I know, this is the only option available: https://github.com/kubernetes/kubernetes/blob/master/pkg/kubeapiserver/admission/initializer.go#L31.

We had updated all our controllers to use versioned clientset instead of internal ones.
Looks like we still need to do that work for our admission controllers.
cc @caesarxuchao to confirm.

Thanks @nikhiljindal. It's ok for admission controllers to use the internal clientset. The admission controller runs in the same process as the apiserver, and iirc objects are supplied by the apiserver to the admission controller, and the objects are already converted to the internal version.

As discussed offline with @caesarxuchao, admission controllers (like this one) also talk to apiserver directly to fetch other resources (like configmaps and events in this case).
Admission controllers should be using versioned clientsets for that.

This PR is fine as is since we do not have a way to provide versioned clients today, but adding support for it should not be too difficult. We can add another method SetVersionedKubeClientSet similar to SetInternalKubeClientSet.
This will become critical as we move admission controllers out of repo without moving the internal API structs.

nikhiljindal · 2017-05-24T04:08:40Z

federation/plugin/pkg/admission/schedulingpolicy/admission.go

+
+func (c *admissionController) Admit(a admission.Attributes) (err error) {
+
+	failClosed, err := c.shouldFailClosed()


if there is no policy defined, then we can just return here. No need to query the policy engine.

For test purposes it's useful to have the fail-open option. Why don't we make this configurable as follows:

By default, if no ConfigMap exists, return here. If ConfigMap exists, fail-closed.

If admission controller configuration includes some flag (e.g., failOpenOnNonExistentPolicy) then current behaviour will be used.

Sorry can you explain more why is fail-open option required for testing?

By default, if no ConfigMap exists, return here. If ConfigMap exists, fail-closed.

This sgtm.

nikhiljindal

Thanks for the PR @tsandall

High level comments:

Use versioned clientset and versioned objects.
Share as much code as possible with existing controllers.
Some discussion on PolicyEngineAPI. Returning the Object instead of annotations is future change compatible when we move from annotations to fields.

nikhiljindal · 2017-05-24T04:13:42Z

cc @sig-federation-pr-reviews

Also update the description to include release notes

tsandall · 2017-05-25T05:20:52Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this

nikhiljindal · 2017-05-26T19:29:02Z

federation/plugin/pkg/admission/schedulingpolicy/admission.go

+	}
+
+	if len(cfg.Kubeconfig) == 0 {
+		return nil, fmt.Errorf("kubeconfig must specify path")


This is not meaningful at all

My bad, I'll improve this.

nikhiljindal · 2017-05-27T00:33:31Z

federation/plugin/pkg/admission/schedulingpolicy/query.go

+	Errors   []string `json:"errors,omitempty"`
+	Resource struct {
+		Metadata struct {
+			Annotations map[string]string `json:"annotations,omitempty"`


Yes, on thinking more I agree with you. This way, we restrict what the policy engine can change. It cannot change anything apart from annotations on the object.
Infact, I think we can restrict it more. Instead of returning a whole map of annotations and we merging them all as is, it should only return cluster selector values.

This is fine as is for now.

nikhiljindal · 2017-05-27T00:36:07Z

federation/plugin/pkg/admission/schedulingpolicy/admission.go

+	// backoff implementation is handled by k8s.io/apiserver/pkg/util/webhook.
+	// The defaultRetryBackoff is used if the admission controller
+	// configuration does not specify one.
+	minRetryBackoff     = time.Millisecond


I assume you are following the pattern of other admission controllers, but it is not clear to me why we need these.
Admin should be allowed to configure any backoff they seem fit.

Indeed, I was just following convention. I'll remove these.

nikhiljindal · 2017-05-27T00:37:13Z

federation/plugin/pkg/admission/schedulingpolicy/admission.go

-	client                   internalclientset.Interface // client to communicate with federation-apiserve
+	client                   internalclientset.Interface // client to communicate with federation-apiserver
+
+	// controls whether policy engine is queried when policy has not been


Whats a scenario where admin should set this to true?

The admin would set this flag to true if they were loading the policies into the engine out-of-band and wanted to exercise the policies for test purposes.

This flag would not be set in production.

In the initial version of the PR this flag was not necessary as the admission controller ALWAYS queried the policy engine and the fail-open vs fail-closed decision was determined by the existence of a ConfigMap in the built-in namespace.

I dont think we need this flag.
Even for testing, admin should create the policies in configmap.

If there is no configmap (i.e policies not defined), then admission controller can just return,
If there is a configmap, then admission controller should call the policy engine and fail in case of an error.

nikhiljindal · 2017-05-27T00:40:14Z

@quinton-hoole or @csbell Will be great if one of you can also take a look at this. Specially the API with policy engine part (query.go)

perotinus · 2017-05-27T00:43:05Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this

nikhiljindal · 2017-06-01T22:56:35Z

mostly looks good.
Will add lgtm when you have fixed open comments and squashed and fixed release notes.

nikhiljindal · 2017-06-01T23:48:24Z

/release-note

/lgtm
/approval

nikhiljindal · 2017-06-02T00:00:26Z

/lgtm

tsandall · 2017-06-02T00:01:25Z

Fixed the bazel build failure. Forgot to run ./hack/update-bazel.sh after fixing @marun's suggestion about runtime.HandleError.

madhusudancs · 2017-06-02T01:03:03Z

/approve

(for hack/)

fejta · 2017-06-02T05:15:51Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this
ref: #46827

nikhiljindal · 2017-06-05T17:21:50Z

/lgtm
/approve
@k8s-bot pull-kubernetes-e2e-kops-aws test this

tsandall · 2017-06-06T16:05:19Z

@k8s-bot pull-kubernetes-e2e-kops-aws test this

nikhiljindal · 2017-06-06T16:31:38Z

/approve

k8s-github-robot · 2017-06-06T18:43:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: madhusudancs, nikhiljindal, tsandall

Associated issue: 39982

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~federation/OWNERS~~ [madhusudancs,nikhiljindal]
~~hack/OWNERS~~ [madhusudancs]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-06-07T07:27:46Z

Automatic merge from submit-queue (batch tested with PRs 46235, 44786, 46833, 46756, 46669)

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 21, 2017

k8s-github-robot assigned ghost and irfanurrehman Apr 21, 2017

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Apr 21, 2017

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 21, 2017

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 21, 2017

ghost assigned nikhiljindal and unassigned irfanurrehman May 1, 2017

ghost reviewed May 1, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 18, 2017

nikhiljindal reviewed May 24, 2017

View reviewed changes

tsandall force-pushed the f8n-scheduling-policy branch from e1d0911 to 962b9c0 Compare May 24, 2017 22:41

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 24, 2017

nikhiljindal reviewed May 27, 2017

View reviewed changes

tsandall force-pushed the f8n-scheduling-policy branch 2 times, most recently from 60cf0fd to 5847b7d Compare May 27, 2017 17:15

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 31, 2017

csbell added the sig/federation label Jun 1, 2017

tsandall force-pushed the f8n-scheduling-policy branch from 3532a88 to ab9669e Compare June 1, 2017 21:46

tsandall force-pushed the f8n-scheduling-policy branch from ab9669e to 79bf550 Compare June 1, 2017 23:06

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed release-note-label-needed labels Jun 1, 2017

tsandall force-pushed the f8n-scheduling-policy branch from 79bf550 to 8a3c637 Compare June 1, 2017 23:59

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 1, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 2, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 2, 2017

calebamiles modified the milestone: v1.7 Jun 2, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 3, 2017

federation: Add admission controller for policy-based placement

470e99c

tsandall force-pushed the f8n-scheduling-policy branch from 8a3c637 to 470e99c Compare June 5, 2017 14:59

k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 5, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 5, 2017

k8s-github-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 5, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 6, 2017

k8s-github-robot merged commit eae59aa into kubernetes:master Jun 7, 2017

		}

		func (c *admissionController) Admit(a admission.Attributes) (err error) {


		func (c *admissionController) Admit(a admission.Attributes) (err error) {

		failClosed, err := c.shouldFailClosed()

federation: Add admission controller for policy-based placement #44786

federation: Add admission controller for policy-based placement #44786

Conversation

tsandall commented Apr 21, 2017 • edited

k8s-reviewable commented Apr 21, 2017

k8s-ci-robot commented Apr 21, 2017

ghost commented Apr 21, 2017

ghost commented May 1, 2017

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsandall May 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikhiljindal left a comment

Choose a reason for hiding this comment

nikhiljindal commented May 24, 2017

tsandall commented May 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikhiljindal commented May 27, 2017

perotinus commented May 27, 2017

nikhiljindal commented Jun 1, 2017

nikhiljindal commented Jun 1, 2017

nikhiljindal commented Jun 2, 2017

tsandall commented Jun 2, 2017

madhusudancs commented Jun 2, 2017

fejta commented Jun 2, 2017

nikhiljindal commented Jun 5, 2017

tsandall commented Jun 6, 2017

nikhiljindal commented Jun 6, 2017

k8s-github-robot commented Jun 6, 2017

k8s-github-robot commented Jun 7, 2017

tsandall commented Apr 21, 2017 •

edited

tsandall May 24, 2017 •

edited